feat: v1 trace protocol implementation #3947
Conversation
- Added msgp struct tags to fields in payloadV1 and traceChunk for serialization. - Introduced EncodeMsg methods for payloadV1 and spanListV1 to support msgp encoding. - Updated field types in traceChunk to enhance compatibility with msgp serialization.
BenchmarksBenchmark execution time: 2025-10-29 18:55:12 Comparing candidate commit 6e71dbd in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 23 metrics, 0 unstable metrics. scenario:BenchmarkParallelMetrics/distribution/get-handle-24
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: 6e71dbd | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
| func checkEndpoint(c *http.Client, endpoint string, protocol float64) error { | ||
| b := []byte{0x90} // empty array | ||
| if protocol == traceProtocolV1 { | ||
| b = []byte{0x80} // empty map |
There was a problem hiding this comment.
Payload v1 is represented by a message pack map, whereas the empty payload in v0.4 was represented by an array. To prevent failures when we send empty data, we need to check for the payload version and send the correct data type.
| // • https://github.com/DataDog/dd-trace-go/pull/475 | ||
| // • https://github.com/DataDog/dd-trace-go/pull/549 | ||
| // • https://github.com/DataDog/dd-trace-go/pull/976 | ||
| type payloadV04 struct { |
There was a problem hiding this comment.
For clarity, the old payload (previously named payload and living in payload.go) is named payloadV04 to represent v0.4. The associated functions are moved to payload_v04.go, and v1 payload functionality is added to payload_v1.go.
mtoffl01
left a comment
mtoffl01
left a comment
There was a problem hiding this comment.
Left some feedback for improvements, but conceptually looks good
| if internal.BoolEnv("DD_TRACE_V1_PAYLOAD_FORMAT_ENABLED", false) { | ||
| c.traceProtocol = traceProtocolV1 | ||
| if t, ok := c.transport.(*httpTransport); ok { | ||
| t.traceURL = fmt.Sprintf("%s%s", c.agentURL.String(), tracesAPIPathV1) | ||
| } | ||
| } else { | ||
| c.traceProtocol = traceProtocolV04 | ||
| } |
There was a problem hiding this comment.
Seems weird that we're modifying httpTransport.traceURL directly for the traceProtocolV1 case, but not for other cases.
Maybe we can modify the newHTTPTransport method to consider tracesAPIPathV1 as well, and move these if/else checks above where we set c.transport.
Like this (pseudocode):
if (DD_TRACE_V1_PAYLOAD_FORMAT_ENABLED) {
c.traceProtocol = traceProtocolV1
} else {
c.traceProtocol = traceProtocolV04
}
if (c.transport == nil) { c.transport = newHTTPTransport(c.agentURL.String(), c.httpClient, c.traceProtocol }
| // safePayload provides a thread-safe wrapper around unsafePayload. | ||
| // safePayload provides a thread-safe wrapper around payload. | ||
| type safePayload struct { | ||
| mu sync.RWMutex | ||
| p *unsafePayload | ||
| p payload |
There was a problem hiding this comment.
I'm slightly confused by *unsafePayload becoming payload. v04 payload was always "unsafe," and v1 payload is also marked as "not safe for concurrent use"
There was a problem hiding this comment.
@mtoffl01 payload is an interface so we can reuse safePayload with the unsafe payloads for both trace protocols.
| if p, ok := span.Context().SamplingPriority(); ok { | ||
| origin = span.Context().origin // TODO(darccio): are we sure that origin will be shared across all the spans in the chunk? | ||
| priority = p // TODO(darccio): the same goes for priority. | ||
| dm := span.context.trace.propagatingTag(keyDecisionMaker) |
There was a problem hiding this comment.
Is it possible any of these fields could be nil and we could hit an invalid memory address error 😄
…v1-serialization' into hannahkm/implement-v1-serialization
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/merge |
|
View all feedbacks in Devflow UI.
This merge request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
Co-authored-by: darccio <[email protected]> Co-authored-by: hannahs.kim <[email protected]>
| func DecodeSpanEvents([]byte, *stringTable) ([]spanEvent, []byte, error) | ||
| func DecodeSpanLinks([]byte, *stringTable) ([]SpanLink, []byte, error) | ||
| func DecodeSpans([]byte, *stringTable) (spanList, []byte, error) | ||
| func DecodeTraceChunks([]byte, *stringTable) ([]traceChunk, []byte, error) |
There was a problem hiding this comment.
Why are we adding these public APIs?
There was a problem hiding this comment.
That's a good catch. @hannahkm Do we need them? I think everything happens inside the same package, right?
There was a problem hiding this comment.
Good point. We don't need them to be public, as we aren't (and probably shouldn't) be calling this functions from anywhere else.
Co-authored-by: darccio <[email protected]>
What does this PR do?
Implements the v1 trace protocol. Includes commits cherry-picked from #3932 since I closed my previous PR.
The code is DISABLED by default. In the future, v1.0 will replace v0.4 as the default protocol.
Motivation
https://datadoghq.atlassian.net/browse/LANGPLAT-750
RFC
Reviewer's Checklist
./scripts/lint.shlocally.Unsure? Have a question? Request a review!